Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TextKit 2 - Implement basic TK2 class and graceful fallback to TK1 #1690

Merged
merged 7 commits into from
Dec 26, 2024

Conversation

charliescheer
Copy link
Contributor

@charliescheer charliescheer commented Dec 21, 2024

Fix

This PR is the first step in implementing Textkit 2 into Simplenote. Why do we need this? Well primarily it is to keep up with the next paradigms in apple development. Also with the addition of Textkit 2 we should get all of the new Apple intelligence features for free. Lastly, by removing the reliance on glyphs and glyph ranges in TextKit 2, we should be able to fix many of the out of range crashes that we sometimes see with multi bit character languages.

Here I have taken the first steps. I have:

  • created the TextKit 2 layout properties if the device can run them
  • layout the text with the same appearance as text in TextKit 1

NOTE: this PR is broken for iOS 16 and older. That is fixed in #1691

Test

  1. Launch the app, open a note
  • Confirm that the text is styled correctly for SN
  • Confirm that if you tap into the text you are able to become first responder
  • Confirm that you can enter text
  • Confirm that after you tap into the text you see this warning in the console UITextView 0x10500fe00 is switching to TextKit 1 compatibility mode because its layoutManager was accessed but the text is still styled

NOTE: When the editor becomes first responder the text will lose formatting because it has fallen back to text kit 1.

Review

(Required) Add instructions for reviewers. For example:

Only one developer and required to review these changes, but anyone can perform the review.

Release

(Required) Add a concise statement to RELEASE-NOTES.txt if the changes should be included in release notes. Include details about updating the notes in this section. For example:
These changes will require an update, but not yet

@charliescheer charliescheer self-assigned this Dec 21, 2024
@charliescheer charliescheer added the [feature] editor Anything related to the editor. label Dec 21, 2024
@dangermattic
Copy link
Collaborator

dangermattic commented Dec 21, 2024

1 Warning
⚠️ View files have been modified, but no screenshot or video is included in the pull request. Consider adding some for clarity.

Generated by 🚫 Danger

@charliescheer charliescheer marked this pull request as ready for review December 21, 2024 23:00
@charliescheer charliescheer added this to the Future milestone Dec 21, 2024
@charliescheer charliescheer changed the title Charlie/textkit 2 round2 mk1 TextKit 2 - Implement basic TK2 class and graceful fallback to TK1 Dec 21, 2024
@charliescheer charliescheer mentioned this pull request Dec 21, 2024
4 tasks
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Dec 21, 2024

Simplenote Prototype Build📲 You can test the changes from this Pull Request in Simplenote Prototype Build by scanning the QR code below to install the corresponding build.
App NameSimplenote Prototype Build Simplenote Prototype Build
Build Numberpr1690-0a48d63-019404fa-4cb0-42b1-9446-32476045f7f8
Version4.55
Bundle IDcom.codality.NotationalFlow.Alpha
Commit0a48d63
App Center BuildSimplenote - Installable Builds #422
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

Copy link
Contributor

@jleandroperez jleandroperez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@charliescheer I spotted a couple issues (while running on an iOS 18 device):

Bug: No Text + Crasher

  1. Add a new Note with a single line
  2. Go back to the Notes List
  3. Open the note again

As a result, the text is not visible. If you attempt to type anything, Simplenote crashes.

ScreenRecording_12-23-2024.12-28-02_1.MP4
Screenshot 2024-12-23 at 12 20 51 PM

Bug: No Style

  1. Add a new Note
  2. Write more than one line

As a result, the H1 style is never applied.

ScreenRecording_12-23-2024.12-29-07_1.MP4

Simplenote/SPFallbackTextContainer.swift Outdated Show resolved Hide resolved
Simplenote/SPFallbackTextContainer.swift Outdated Show resolved Hide resolved
@charliescheer charliescheer changed the base branch from trunk to feature/text-kit-2 December 26, 2024 20:53
@charliescheer
Copy link
Contributor Author

So the problem with the crashing was how the fallback was being setup. It could mean that there was no text when changes were applied, so the ranges would always be out of bounds.

Discussed in slack and decided instead to approach this in a feature branch rather than building a fallback. In theory if we are not using TK1 properties then we don't need to fallback. We don't use them too much already, so if we patch over those for iOS 17 and higher then we should be fine to stay in TK2 always. Updating the PR to reflect that

@charliescheer
Copy link
Contributor Author

charliescheer commented Dec 26, 2024

As for the editing of the header in a new note, this PR is just setting up the TK2 properties and displaying existing notes, it doesn't yet handle editing. That will come later.

textLayoutManager.textContainer = container

} else {
layoutManager.addTextContainer(container)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick note: layoutManager may not be init'ed by the time it's called here

//
extension SPTextView: NSTextContentStorageDelegate {
public func textContentStorage(_ textContentStorage: NSTextContentStorage, textParagraphWith range: NSRange) -> NSTextParagraph? {
guard let originalText = textContentStorage.textStorage?.attributedSubstring(from: range) as? NSMutableAttributedString else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing .mutabelCopy() here

Copy link
Contributor

@jleandroperez jleandroperez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed over Slack, the mutableCopy() change is coming up in a follow up.

Please feel free to :shipit: once the NSLayoutManager initialization is fixed (fallback flow!).

Nice work @charliescheer !!!

@charliescheer charliescheer merged commit c05ea3b into feature/text-kit-2 Dec 26, 2024
7 of 9 checks passed
@charliescheer charliescheer deleted the charlie/textkit-2-round2-mk1 branch December 26, 2024 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[feature] editor Anything related to the editor.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants